Fixes handling of numeric strings in span tag values #436
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matus-m
force-pushed
the
fix-numericstring-handling
branch
from
adb8dcf
to
8b7b0f8
Compare
Codecov Report
@@ Coverage Diff @@
## master #436 +/- ##
==========================================
- Coverage 90.98% 90.94% -0.05%
==========================================
Files 177 177
Lines 4106 4108 +2
Branches 991 992 +1
==========================================
Hits 3736 3736
- Misses 327 329 +2
Partials 43 43
Continue to review full report at Codecov.
|
tiffon
approved these changes
Aug 9, 2019
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.tsx
Outdated
Show resolved
Hide resolved
matus-m
force-pushed
the
fix-numericstring-handling
branch
4 times, most recently
from
57abda4
to
066ed83
Compare
Signed-off-by: Matus Majchrak <[email protected]>
matus-m
force-pushed
the
fix-numericstring-handling
branch
from
19430b0
to
510f77e
Compare
Signed-off-by: Matus Majchrak <[email protected]>
|
alright, could you, please, give it one more try? |
tiffon
suggested changes
Aug 11, 2019
|
|
||
| function parseIfComplexJson(value: any) { | ||
| // if the value is a string representing actual json object or array, then use json-markup | ||
| if (typeof value === 'string' && value.match(jsonObjectOrArrayStartRegex)) { |
There was a problem hiding this comment.
I think this can be jsonObjectOrArrayStartRegex.test(value) to avoid generating match data?
| if (typeof value === 'string' && value.match(jsonObjectOrArrayStartRegex)) { | ||
| // otherwise just return as is | ||
| try { | ||
| console.log('parsing JSON'); |
There was a problem hiding this comment.
Is this log statement a left-over?
Signed-off-by: Matus Majchrak <[email protected]>
matus-m
force-pushed
the
fix-numericstring-handling
branch
from
1c038e6
to
4b61e23
Compare
🎉 Thanks for contributing!! 🎉 |
|
Pleasure is all mine, well setup project and great feedback. |
vvvprabhakar
pushed a commit
to vvvprabhakar/jaeger-ui
that referenced
this pull request
Jul 5, 2021
Fixes handling of numeric strings in span tag values Signed-off-by: vvvprabhakar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Matus Majchrak [email protected]
Fixes the rendering of span tag values that contain numeric strings
Hi this is my first PR to this project so bear with me :)
Resolves #396
Which problem is this PR solving?
As stated in bug #396, span tags containing numeric strings will be converted to actual numbers and rendered in scientific notation if they are large enough.
Short description of the changes
As hinted by @tiffon, this commit limits the json conversion of tag values only to cases, when the value is an actual string and is not a primitive type(i.e. only if it is object or array)
The one open point from my side, is what would be a meaningful test for the rendering of tag value containing real json object(if it is required) as I had some trouble creating an elegant test case that would verify the
dangerouslySetInnerHTMLcontents(right now I verify it by call tohtml()and then string matching...) and what role should tagtypeattribute play in the rendering:e.g.
given span tag payload:
Not sure if this is a valid case.
In any case, looking forward to your feedback.